patina_dxe_core: Add DxeDispatch service for driver dispatch#1421
patina_dxe_core: Add DxeDispatch service for driver dispatch#1421kat-perez wants to merge 1 commit intoOpenDevicePartnership:mainfrom
Conversation
✅ QEMU Validation PassedAll QEMU validation jobs completed successfully.
Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/23465890837 Boot Time to EFI Shell
Dependencies
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
2fa66cb to
b8285d5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b8285d5 to
527b860
Compare
patina_dxe_core/src/lib.rs
Outdated
| /// Performs a single DXE driver dispatch pass. | ||
| /// | ||
| /// Returns `true` if any drivers were dispatched, `false` if no drivers remain. | ||
| pub fn dispatch(&'static self) -> Result<bool> { |
There was a problem hiding this comment.
I don't think making this public makes sense. @Javagedes do you have thoughts on this? Feels like a different paradigm is needed.
There was a problem hiding this comment.
I could move this to the SDK instead?
Like this at sdk/patina/src/pi/dxe_services.rs?
/// Interface for dispatching DXE drivers.
///
/// Boot orchestrators use this to interleave controller connection
/// with driver dispatch.
pub trait DxeServices: Send + Sync + 'static {
/// Run a single dispatch pass. Returns `true` if any drivers were dispatched.
fn dispatch(&self) -> crate::error::Result<bool>;
}
There was a problem hiding this comment.
Yeah, I don't think we want to make this public outside of the core. It leaks containment of functionality that the platform's top level main() function should not have access to, which could result in platforms doing bad things.
Based off of the current usage:
impl DxeServices for CoreDxeServices {
fn dispatch(&self) -> patina::error::Result<bool> {
CORE.dispatch()
}
}
// Boot orchestration with connect-dispatch interleaving
add.component(BootDispatcher::new(
SimpleBootManager::new(
BootConfig::new(create_boot_path())
.with_failure_handler(|| log::error!("Boot failed: all boot options exhausted")),
),
CoreDxeServices,
));I think one of these two options better lean into the patina component model via dependency injection:
- Create a new service trait defined in the
patinacrate for either (1) only dispatch or (2) the entire dxe services table, and implemented in the core (Similar to the CoreMemoryManager), then consume it through dependency injection - Do the same as 1, but instead do it for configuration table functionality, then you can grab the dxe services configuration table and call dispatch from dxe_services
There was a problem hiding this comment.
Thanks, I'll go with (1.) by making a new service trait instead. Would be something like CoreDxeDispatch, implemented like CoreMemoryManager
There was a problem hiding this comment.
The platform binary will never touch dispatch() directly
527b860 to
386036d
Compare
c8162fd to
762aa1d
Compare
|
@os-d @Javagedes this is ready for another review. thanks! |
| pub trait DxeDispatch { | ||
| /// Performs a single DXE driver dispatch pass. | ||
| /// | ||
| /// Returns `true` if any drivers were dispatched, `false` if no drivers remain. |
There was a problem hiding this comment.
I believe false means no drivers were dispatched, not necessarily that there are no more remaining drivers.
os-d
left a comment
There was a problem hiding this comment.
I think the overall approach is reasonable, we would want the other dispatch services also defined at some point, but I think it is okay to defer those until a use case arises
| impl CoreDxeDispatch { | ||
| /// Create a new dispatch service with the given dispatch function. | ||
| pub(crate) fn new(dispatch_fn: fn() -> Result<bool>) -> Self { | ||
| Self(dispatch_fn) |
There was a problem hiding this comment.
Why is this configurable? Can we not just always use the pi_dispatcher?
There was a problem hiding this comment.
Can't the whole trait just be mocked?
There was a problem hiding this comment.
Fair :) I concur with @os-d we should just hardcode in the usage of the core instead of having the wrapper like this.
Also if you update init_memory to be &'static self (which I think is fair), then I think you could even do something like this:
pub(crate) struct CoreDxeDispatch<P: PlatformInfo>(&'static Core<P>);
// ...
component_dispatcher.add_service(dxe_dispatch_service::CoreDxeDispatch::new(self);I'm not 100% confident in that though.
7469952 to
00af474
Compare
df163af to
1d334a4
Compare
1d334a4 to
6a617c3
Compare
6a617c3 to
0fae68c
Compare
Description
Add a
DxeDispatchservice trait in the SDK and aCoreDxeDispatchimplementation in patina_dxe_core that delegates to the PI dispatcher.
The service is registered alongside other core services (MemoryManager,
PerfTimer, etc.) and consumed via dependency injection by components
that need to trigger driver dispatch passes.
How This Was Tested
BootDispatcher+SimpleBootManagerconsuming the service via DIIntegration Instructions
The
DxeDispatchservice is registered automatically by the DXE core.Components consume it via dependency injection: